fix(plugin-harness): add no-op process_deferred_updates to test double#391
Merged
Merged
Conversation
The safety harness's VisualTestDisplayManager (base of BoundsCheckingDisplayManager) doesn't implement process_deferred_updates(), which 5 first-party ledmatrix-plugins call unconditionally between set_scrolling_state() and their scroll-position update: news, odds-ticker, ledmatrix-leaderboard, stock-news, and ledmatrix-stocks. Any of them fails the harness with AttributeError the moment it's touched (surfaced when ledmatrix-plugins#177 had to add a local hasattr guard in ledmatrix-stocks just to pass CI). Add the method as a no-op, mirroring the existing "no-op for testing" pattern already used for set_scrolling_state, so these plugins render under the harness without every touching PR needing its own guard.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesVisual display deferred updates
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The plugin-safety harness's
VisualTestDisplayManager(base class ofBoundsCheckingDisplayManager) doesn't implementprocess_deferred_updates().Five first-party plugins in
ledmatrix-pluginscall it unconditionally,right alongside
set_scrolling_state(), before updating scroll position:news,odds-ticker,ledmatrix-leaderboard,stock-news, andledmatrix-stocks. Any of them fails the harness withAttributeErrorthe instant it's touched, even though the call is completely safe against
the real production
display_manager.py(which has always had thismethod).
This surfaced concretely in ledmatrix-plugins#177,
which had to add a local
hasattr(self.display_manager, "process_deferred_updates")guard in
ledmatrix-stocks/manager.pyjust to get CI green — a workaroundfor this exact gap, not a real plugin bug. The other 4 plugins are still
unguarded and will hit the same failure the next time any of them is
touched.
Fix
Add
process_deferred_updates()toVisualTestDisplayManageras a no-op,mirroring the existing "no-op for testing" pattern already used a few lines
above for
set_scrolling_state(). This double has no deferred-update queueto process, so there's nothing to do — the point is just to stop raising
AttributeErrorfor callers that assume the method exists (as productioncode always can).
Test plan
test_process_deferred_updates_is_nooptotest/plugins/test_visual_rendering.py,next to the existing
test_scrolling_statetest, asserting the calldoesn't raise and doesn't disturb scrolling state.
python3 -m py_compileon both touched files.test/plugins/test_harness.py,test_visual_rendering.py,and
test_plugin_matrix.pyto confirm none of them do exhaustivemethod-set introspection that this addition would break.
test.yml→pytest test/plugins/test_harness.py test/plugins/test_visual_rendering.py test/plugins/test_plugin_matrix.py)will exercise this on the PR.
Follow-up (not in this PR)
Once this merges, the local
hasattrguard added inledmatrix-plugins#177becomes unnecessary (though harmless to leave). Theother 4 affected plugins (news, odds-ticker, leaderboard, stock-news) need
no changes at all — they'll simply stop failing the harness next time
they're touched.
Summary by CodeRabbit
Bug Fixes
Tests